container: T7736: give container veths a deterministic host_interface_name - #5352
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughContainer networking now assigns deterministic host-side interface names for non-macvlan networks, validates collisions, emits explicit Podman network options, and exposes interface status commands. The package now requires Podman 5.8, netavark 1.14.0, and aardvark-dns 1.14.0. ChangesContainer networking
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/conf_mode/container.py`:
- Around line 364-373: The get_container_host_ifname function must avoid
collisions caused by truncating container names to 15 characters. Generate a
deterministic host interface name using a bounded hash suffix (or otherwise
guarantee uniqueness), and update verify() to reject any duplicate generated
names if needed. Add a regression test covering distinct long names such as
abcdefghij-1 and abcdefghij-2.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 87df2d00-9496-48eb-aa88-ce7b0312771f
📒 Files selected for processing (2)
debian/controlsrc/conf_mode/container.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: build_iso
- GitHub Check: codeql-analysis-call / Analyze (python)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
⚠️ CI failures not shown inline (2)
GitHub Actions: Python Lint (Darker + Ruff) / darker-ruff-lint _ darker-ruff-lint: container: T7736: give container veths a deterministic host_interface_name
Conclusion: failure
##[group]Run echo "### 🧪 Lint Results"
�[36;1mecho "### 🧪 Lint Results"�[0m
�[36;1mdarker_failed="1"�[0m
�[36;1mgraylint_failed=""�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Darker** check failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Darker** check passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Graylint (ruff check)** failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Graylint (ruff check)** passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" || "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "::error::One or more linters failed. See above for details."�[0m
GitHub Actions: Python Lint (Darker + Ruff) / 0_darker-ruff-lint _ darker-ruff-lint.txt: container: T7736: give container veths a deterministic host_interface_name
Conclusion: failure
##[group]Run echo "### 🧪 Lint Results"
�[36;1mecho "### 🧪 Lint Results"�[0m
�[36;1mdarker_failed="1"�[0m
�[36;1mgraylint_failed=""�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Darker** check failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Darker** check passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Graylint (ruff check)** failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Graylint (ruff check)** passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" || "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "::error::One or more linters failed. See above for details."�[0m
🧰 Additional context used
📓 Path-based instructions (2)
src/conf_mode/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and located in
src/conf_mode/
Files:
src/conf_mode/container.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use ruff 0.6.4 for Python linting with configuration inruff.tomlat repository root
Use pylint to check for W0611 (unused imports) violations in Python code
Use darker for code formatting in Python files
Use nose2 for Python testing with configuration innose2.cfgat repository root
Files:
src/conf_mode/container.py
🔍 Remote MCP
Relevant Podman docs for this PR:
host_interface_nameis documented for bridge networks:--network=bridge:host_interface_name=<name>sets the external/host-side interface name. citeContext7::Context7_query-docs::- Podman’s network docs also say
interface_nameis the container-side name, whilehost_interface_nameis the outside/container-host name. citeContext7::Context7_query-docs:: - For macvlan/ipvlan, Podman docs describe the option as selecting the parent host device (
--opt parent=.../--interface-name), not a host-side veth pair. That supports the PR’s choice to avoid assigninghost_interface_namefor macvlan attachments. citeContext7::Context7_query-docs::
I did not find version-provenance for when host_interface_name was introduced in the retrieved docs.
🔇 Additional comments (3)
src/conf_mode/container.py (2)
526-540: LGTM!Also applies to: 551-552, 575-575, 655-661
376-376: 🎯 Functional CorrectnessNo additional callers need changes —
src/conf_mode/container.py:661already passesnetwork_config, and no othergenerate_run_arguments()calls exist.> Likely an incorrect or invalid review comment.debian/control (1)
335-335: 🎯 Functional Correctness
podman (>= 5.8)is sufficient indebian/control:335
host_interface_nameis available for bridge/user-defined network use, so no version bump is needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@smoketest/scripts/cli/test_container.py`:
- Around line 267-293: The test_long_name_host_interface_uniqueness test
currently verifies only PID existence, so it must also validate the
host-interface contract and container state. After cli_commit, obtain each
container’s actual host-side interface through the existing Podman or systemd
inspection mechanism, assert both interfaces are present and distinct, and
verify each expected container is running via Podman or systemd rather than
relying on /proc/<pid> alone.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: bc675035-0c27-46ef-8715-59a0a0e3f615
📒 Files selected for processing (3)
debian/controlsmoketest/scripts/cli/test_container.pysrc/conf_mode/container.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
🚧 Files skipped from review as they are similar to previous changes (1)
- debian/control
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: build_iso
- GitHub Check: codeql-analysis-call / Analyze (c-cpp)
- GitHub Check: codeql-analysis-call / Analyze (python)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
⚠️ CI failures not shown inline (2)
GitHub Actions: Python Lint (Darker + Ruff) / darker-ruff-lint _ darker-ruff-lint: container: T7736: give container veths a deterministic host_interface_name
Conclusion: failure
##[group]Run echo "### 🧪 Lint Results"
�[36;1mecho "### 🧪 Lint Results"�[0m
�[36;1mdarker_failed="1"�[0m
�[36;1mgraylint_failed=""�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Darker** check failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Darker** check passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Graylint (ruff check)** failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Graylint (ruff check)** passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" || "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "::error::One or more linters failed. See above for details."�[0m
GitHub Actions: Python Lint (Darker + Ruff) / 0_darker-ruff-lint _ darker-ruff-lint.txt: container: T7736: give container veths a deterministic host_interface_name
Conclusion: failure
##[group]Run echo "### 🧪 Lint Results"
�[36;1mecho "### 🧪 Lint Results"�[0m
�[36;1mdarker_failed="1"�[0m
�[36;1mgraylint_failed=""�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Darker** check failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Darker** check passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Graylint (ruff check)** failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Graylint (ruff check)** passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" || "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "::error::One or more linters failed. See above for details."�[0m
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use ruff 0.6.4 for Python linting with configuration inruff.tomlat repository root
Use pylint to check for W0611 (unused imports) violations in Python code
Use darker for code formatting in Python files
Use nose2 for Python testing with configuration innose2.cfgat repository root
Files:
smoketest/scripts/cli/test_container.pysrc/conf_mode/container.py
smoketest/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Runtime smoketests must be located under
smoketest/and use nose2 framework
Files:
smoketest/scripts/cli/test_container.py
src/conf_mode/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and located in
src/conf_mode/
Files:
src/conf_mode/container.py
🧠 Learnings (3)
📚 Learning: 2026-05-26T06:03:59.703Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:206-207
Timestamp: 2026-05-26T06:03:59.703Z
Learning: In VyOS smoketests that verify processes running inside a VRF using iproute2, remember that `ip vrf pids <vrf>` outputs one entry per line as `<pid> <process_name>` (e.g., `300431 nginx`), not PIDs alone. Therefore, assertions should check for the presence of the expected process name in the command output (e.g., `assertIn(PROCESS_NAME, cmd(f'ip vrf pids {vrf}'))`) rather than trying to match PID-only output.
Applied to files:
smoketest/scripts/cli/test_container.py
📚 Learning: 2026-05-26T06:04:29.163Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:118-120
Timestamp: 2026-05-26T06:04:29.163Z
Learning: In VyOS smoketest scripts under `smoketest/scripts/cli/`, it is intentional to call `self.cli_delete(['vrf'])` in both `setUpClass` and `tearDown` to wipe the entire VRF subtree and ensure a clean slate. During code review, do not recommend narrowing the delete to specific VRF identifiers or name subsets (e.g., `['vrf', 'name', 'mgmt']`)—the broad teardown behavior is the established project-wide pattern for these tests.
Applied to files:
smoketest/scripts/cli/test_container.py
📚 Learning: 2026-06-29T12:13:51.293Z
Learnt from: andamasov
Repo: vyos/vyos-1x PR: 5298
File: smoketest/scripts/cli/test_vpp.py:0-0
Timestamp: 2026-06-29T12:13:51.293Z
Learning: When reviewing vyos-1x code that parses or asserts VPP CLI output (e.g., smoketest CLI tests and VPP op-mode code), do not flag the token spelling "Forwrd" / "U-Forwrd" as a typo. It is intentionally preserved verbatim from the upstream VPP CLI text shown by commands like `vppctl show bridge-domain ... detail`. This misspelling is centrally allowlisted (vyos/.github#153) for that specific VPP-CLI context, so typo-review comments should exclude "Forwrd" when it originates from that VPP output.
Applied to files:
smoketest/scripts/cli/test_container.py
🪛 ast-grep (0.44.1)
smoketest/scripts/cli/test_container.py
[warning] 289-289: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(PROCESS_PIDFILE.format(name))
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🔍 Remote MCP Context7
Additional review context
- Podman documents
host_interface_namespecifically for bridge networking as the interface name outside the container;interface_nameis the container-side name. - Podman supports multiple networks by repeating
--network; each network may include its own options. - For macvlan/ipvlan networks, the host-related interface setting identifies the parent device rather than a host-side veth pair.
- The retrieved documentation does not establish when
host_interface_namewas introduced or confirm its availability specifically starting with Podman 5.8.
🔇 Additional comments (4)
src/conf_mode/container.py (4)
162-174: 🎯 Functional CorrectnessDo not equate “non-macvlan” with bridge without enforcing that contract.
These changes treat every non-macvlan network as a paired-veth network. Podman’s
host_interface_nameis bridge-specific; for ipvlan or similar parent-based networks, the host-related setting refers to the parent device. Confirm the schema permits only bridge/macvlan here, or reject/classify other types before reserving and emitting this option.Also applies to: 549-563
Source: MCP tools
378-396: 🎯 Functional CorrectnessMake the interface-length check byte-aware.
len(prefix)counts Unicode code points, while Linux interface limits are byte-based. If container names are not explicitly ASCII-only, a non-ASCII name can pass the 15-character check and still produce an invalid Podman interface name. Enforce ASCII names or truncate/check encoded bytes.
549-563: 🩺 Stability & AvailabilityVerify Podman 5.8 supports
host_interface_name.The package requires Podman
>=5.8, but the supplied documentation does not establish when this option was introduced. Confirm the minimum supported version accepts repeated--network name:host_interface_name=...; otherwise generated units may fail at startup on supported systems.Source: MCP tools
125-125: LGTM!Also applies to: 574-575, 598-598, 678-684
| def test_long_name_host_interface_uniqueness(self): | ||
| # T7736: the deterministic host-side veth interface name derived | ||
| # from a container name is truncated to fit IFNAMSIZ. Two distinct | ||
| # but similarly-prefixed long names must not truncate to the same | ||
| # interface name - Podman would then refuse to attach the second | ||
| # container's network, and its systemd unit would fail to start. | ||
| net_name = 'longiftest' | ||
| prefix = '192.0.2.0/24' | ||
| name_1 = 'abcdefghij-1' | ||
| name_2 = 'abcdefghij-2' | ||
|
|
||
| self.cli_set(base_path + ['network', net_name, 'prefix', prefix]) | ||
| self.cli_set(base_path + ['name', name_1, 'image', busybox_image]) | ||
| self.cli_set(base_path + ['name', name_1, 'network', net_name, 'address', str(ip_interface(prefix).ip + 2)]) | ||
| self.cli_set(base_path + ['name', name_2, 'image', busybox_image]) | ||
| self.cli_set(base_path + ['name', name_2, 'network', net_name, 'address', str(ip_interface(prefix).ip + 3)]) | ||
| self.cli_commit() | ||
|
|
||
| # Both containers run a "conmon" process at once, so checking by | ||
| # process name alone can't distinguish which container it belongs | ||
| # to - verify each container's own recorded PID is still alive | ||
| for name in (name_1, name_2): | ||
| pid = 0 | ||
| with open(PROCESS_PIDFILE.format(name)) as f: | ||
| pid = int(f.read()) | ||
| self.assertTrue(os.path.exists(f'/proc/{pid}')) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the host-interface contract, not only PID existence.
This test can pass even if host_interface_name is removed: Podman may allocate two distinct vethN interfaces automatically. /proc/<pid> existence also does not prove that the expected container is running. Assert each container’s actual host-side interface is present and distinct, and validate container state through Podman or systemd.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 289-289: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(PROCESS_PIDFILE.format(name))
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@smoketest/scripts/cli/test_container.py` around lines 267 - 293, The
test_long_name_host_interface_uniqueness test currently verifies only PID
existence, so it must also validate the host-interface contract and container
state. After cli_commit, obtain each container’s actual host-side interface
through the existing Podman or systemd inspection mechanism, assert both
interfaces are present and distinct, and verify each expected container is
running via Podman or systemd rather than relying on /proc/<pid> alone.
|
What do you think about rather than using a unique hash per name > 15 characters, you truncate in the middle using something like this?: Then it'll keep the uniqueness of the name, while still being obvious to the user which interface is owned by which container. Without that, the user would have to dig to find which interface belongs to homeassistant1, so it'd be annoying to try to use in something like firewall rules. |
This is a compelling idea as my containers ar also numbered in an identical pattern like |
|
@l0crian1 I see one disadvantage of this approach as some container instances could result in the same interface name. My idea is to combine both approaches. use Moved this to DRAFT to implement this change. |
|
Updated PR which also adds a dependency on |
…_name Podman's default "vethN" auto-naming for a container's host-side veth can collide with VyOS's own "virtual-ethernet vethN" interfaces. Bump the minimum Podman dependency to 5.8 (which supports "host_interface_name" network connect option) and use it to name every non-macvlan container network attachment "veth-<container name>" instead, eliminating the collision by construction. Container names too long to fit are shortened to a recognizable prefix plus a short hash of the full name; verify() rejects the rare case where two containers still generate the same interface name. Add "show container interface" to display the resulting name-to-container mapping.
…avark aardvark-dns ships in lockstep with netavark (both 1.14.0 on the box this was verified against), so pin it to the same >=1.14.0 minimum required for Podman's "host_interface_name" network option to actually take effect.
|
I think using ConfigQuery for the What do you think of just redoing some of the work that is done in the conf_mode script, like checking length and creating hash if the length exceeds character limits? Maybe use something like these to return necessary lists (of course you can also do the JSON parsing in python if desired): Get list of networks - This avoids host networking and macvlan networksExample:Get a list of containers with network attachment, and return dict with container name:Example:Then it's as simple as:
As an alternative, you could probably iterate over the containers, and do |
There was a problem hiding this comment.
Re-approving, following changes since initial approval: (1) minor refactoring of conf-mode (2) addition of op-mode command; tested on built image.
PR includes comprehensive smoketests.
I'll leave it to @c-po to consider the most recent suggestion of @l0crian1 regarding the known issue of overhead of ConfigQuery.config_dict on large configs (since it requires parsing/instantiation of the full Config for what should be a simple op-mode command). Recall also the hacky workaround of using op_mode_config_dict from configquery.py which limits the parsing to the path in question, and consequently scales better; it may be used here since default values are not needed. I'm not advocating it over a simpler solution, simply a reminder of alternatives.
|
Tick the box to add this pull request to the merge queue (same as
|
sever-sever
left a comment
sever-sever
left a comment
There was a problem hiding this comment.
Deterministic veth naming looks correct, tests pass, no blockers to merge.
vyos-bot
Bot
added
mirror-initiated
|
CI integration ❌ failed! Details
|
4 participants
Change summary
Podman's default
vethNauto-naming for a container's host-side veth can collide with VyOS's ownvirtual-ethernet vethNinterfaces.Bump the minimum Podman dependency to 5.8 (which supports the "host_interface_name" network connect option) and use it to name every non-macvlan container network attachment
veth-<container name>instead, eliminating the collision by construction.Types of changes
Related Task(s)
https://vyos.dev/T7736
Related PR(s)
How to test / Smoketest result
Checklist: